Skip to content

Don't DM the PR author for every inline comment in a submitted review - #1050

Merged
avasconcelos114 merged 1 commit into
mattermost:masterfrom
nickmisasi:fix-review-comment-dm-fanout
Sep 1, 2026
Merged

Don't DM the PR author for every inline comment in a submitted review#1050
avasconcelos114 merged 1 commit into
mattermost:masterfrom
nickmisasi:fix-review-comment-dm-fanout

Conversation

@nickmisasi

@nickmisasi nickmisasi commented Aug 31, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Submitting a GitHub review with multiple inline comments was sending the PR author one DM per comment, in addition to the existing review-submitted DM.
  • That fan-out came from #1010, which added author DMs for pull_request_review_comment events. GitHub emits one of those webhooks per inline comment when a review is submitted.
  • Skip the author DM for top-level review comments (in_reply_to_id unset); those are already covered by handlePullRequestReviewNotification. Still DM for thread replies, and still notify on @mentions.

QA Notes

  • Submit a review with several new inline comments (not replies): the PR author should get one GitHub-bot DM for the review, not one per comment.
  • Reply on an existing review thread: the PR author should still get a DM for that reply.
  • @mention someone who is not the PR author in an inline comment: they should still get a mention DM.
  • Edit or delete a review comment: no new "commented" DM (existing MM-68829 Improving UX for DMs sent on PR comments #1010 behavior).

Release Note

Fixed GitHub bot DMs so submitting a review with multiple inline comments notifies the PR author once instead of once per comment.

Made with Cursor

Change Impact: 🟡 Medium

Reasoning: The change modifies user-facing notification behavior in the webhook handler. Tests cover top-level comments and thread replies, and the change has a narrow blast radius.

Regression Risk: Medium. Incorrect reply detection could suppress valid author notifications or create duplicate notifications. Existing tests cover the main paths.

** QA Recommendation:** Perform targeted manual QA for submitted reviews with top-level comments, thread replies, edited comments, deleted comments, and @mentions. Skipping manual QA presents a low-to-medium risk because automated coverage exists.

Generated by CodeRabbitAI

@nickmisasi @cursoragent
Don't DM the PR author for every inline comment in a submitted review.
eaffaf5 
GitHub already emits a single pull_request_review event for the submission; top-level review comments are covered by that notification, so only thread replies should send a separate author DM.

Co-authored-by: Cursor <cursoragent@cursor.com>
@nickmisasi
nickmisasi requested a review from a team as a code owner August 31, 2026 20:02
@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0560af45-c851-4f40-a925-0cc209918a99

📥 Commits

Reviewing files that changed from the base of the PR and between 373a255 and eaffaf5.

📒 Files selected for processing (3)
  • server/plugin/test_utils.go
  • server/plugin/webhook.go
  • server/plugin/webhook_test.go

Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.

📝 Walkthrough

Walkthrough

The webhook now sends review comment author notifications only for threaded replies. A new test helper creates reply events, and tests cover threaded replies, top-level comments, unmapped authors, and muted senders.

Changes

Review comment routing

Layer / File(s) Summary
Thread reply notification behavior
server/plugin/test_utils.go, server/plugin/webhook.go, server/plugin/webhook_test.go		 The test helper sets Comment.InReplyTo. The webhook skips top-level review comments. Tests cover threaded reply notifications and related suppression cases.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to eaffa

This localized change prevents duplicate author DMs when a review contains multiple inline comments while preserving notifications for replies and mentions. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: avasconcelos114, jgheithcock

Poem

A rabbit replies in a review thread,
With comment IDs neatly spread.
Top-level notes take another route,
While muted senders stay quiet throughout.
Tests hop along where replies are read.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preventing duplicate direct messages to the pull request author for inline comments in a submitted review.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@nickmisasi nickmisasi self-assigned this Aug 31, 2026
avasconcelos114
avasconcelos114 approved these changes Sep 1, 2026
View reviewed changes

@avasconcelos114 avasconcelos114 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and for flagging this!

@avasconcelos114
avasconcelos114 merged commit 1f3cde7 into mattermost:master Sep 1, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avasconcelos114 avasconcelos114 avasconcelos114 approved these changes
@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

@nickmisasi nickmisasi

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nickmisasi @avasconcelos114